-
Notifications
You must be signed in to change notification settings - Fork 218
Migration of Product Collection and Product Button blocks to the new store()
API
#11558
Migration of Product Collection and Product Button blocks to the new store()
API
#11558
Conversation
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
Because the animation was broken.
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
…-refactor-product-button-with-new-store-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how the code gets much simpler with these changes, good job @luisherranz!
The code changes look good to me and I did some smoke testing and everything is working as expected. However, I don't have much experience in how the Interactivity API worked before, so I would also like if @gigitux can take a look and give his approvall.
By the way, before merging there will need to be some changes to the PR itself: WooCommerce Visibility
should be WooCommerce Core
and we should add a type
label (I guess type: enhancement
?).
} ); | ||
return getTextButton( | ||
context.addToCartText, | ||
state.inTheCartText!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads-up that there is an ESLint warning in this line and L121.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your precious work and the helpful video!
I just added a NIT, feel free to skip it! However, I noticed two things:
- A Circular dependency is detected:
- Currently, there is an E2E test that fails: "21 [blockTheme] › product-collection/product-collection.block_theme.spec.ts:29:2 › Product Collection › Renders product collection block correctly with 9 items (27.0s)". I already re-run it, but it failed again:
@@ -168,20 +164,27 @@ protected function render( $attributes, $content, $block ) { | |||
$this->prevent_cache(); | |||
} | |||
|
|||
$div_directives = 'data-wc-context=\'' . wp_json_encode( $context, JSON_NUMERIC_CHECK ) . '\''; | |||
$interactive = array( | |||
'namespace' => 'woocommerce/product-button', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: We could use using the variables to compose the namespace:
'namespace' => 'woocommerce/product-button', | |
'namespace' => $this->namespace . "/" . $this->block_name, |
We can turn CHECK_CIRCULAR_DEPS off in the Interactivity API config. |
I didn't check the code, but circular dependency could introduce some issues that could be hard to find/debug. I would be more oriented to fix the circular dependency, but it is just an opinion based on my past experience. |
Oh, sorry, I didn't explain myself well enough. I wouldn't suggest changing how you manage your code. But that warning comes from the Interactivity API runtime, so it's ok to remove that circle dep check in the specific Webpack config of the Interactivity API runtime. It won't affect any Woo code. |
Yeah, I got it! |
Thanks, Luigi. For now, we're trying to make things work and want to keep iterating fast. We'll make them pretty after we move them to Gutenberg and we are about to merge them with WP Core 🙂 |
I've updated the visibility and the type label. Feel free to modify them as needed. 🙂 BTW, this branch uses another PR as the base branch, but I added the changes just in case we decide to merge the base branch first to |
With 1a3d9ca I fixed the E2E test failure. For more details, check this issue (we should use
Thanks for the clarification. It makes sense 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for your work!
…-refactor-product-button-with-new-store-api
* Migrate Product Gallery block to new Interactivity API store * Fix some references * Add missing data-wc-interactive * Fix an additional namespace * Remove unnecessary click handler * Dialog working * Refactor action names * Reindex PHP array There was some missing indexes, which turned the array into an object in JS. * Remove unused event handlers * Move next/previous logic to external function * Move StorePart util to the types folder * Rename namespace to `woocommerce/product-gallery` * Undo product collection namespace renaming * Remove unnecessary namespace * Don't hide the large image on page load * Minor refactorings * Fix eslint error * Fix php cs errors with spacing and double arrows alignment * Disable no-use-before-define rule for eslint * Disable @typescript-eslint/ban-types rule for eslint * Fix parsed context error in e2e tests * Fix context parser for Thumbnail image * Move store to the top of the frontend file * Add interactivity api utils to the @woocommerce/utils alias * Replace deprecated event attribute --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: David Arenas <david.arenas@automattic.com> Co-authored-by: roykho <roykho77@gmail.com>
Replace with an interactive block that calls `actions.sel...Replace with an interactive block that calls `actions.selectImage`.
woocommerce-blocks/assets/js/blocks/product-gallery/frontend.tsx Lines 83 to 94 in c10c777
🚀 This comment was generated by the automations bot based on a
|
9ea4bfe
into
interactivity-api-new-store-api
I've merged it even though some tests did not pass. 🤔 To be honest, I'm wondering if they were failing because of a real issue in the code, because everything was green in #11721 and that branch already contained all the changes from this one (#11558), so they should be equivalent once it was merged here. |
* Sync Interactivity API code with Gutenberg * New store() API * Store raw actions * Update wc-interactivity-store implementation * Replace `wc_store` with `wc_initial_state` * Parse and populate initial state * Allow store parts in `store()` * Accept namespaces in directive paths * Add $$namespace to directives' object values * Make namespace parsing more robust * Use DeepPartial type for store parts * Do not pass `rawStore` to `afterLoad` callbacks * Simplify `store()` a bit * Implement `privateStore()` * Sync context directive with Gutenberg * Refactor scope and extract getters per scope * Add namespace to getters and actions * Remove current privateStore implementation * Remove `afterLoad` option from `store` * Use same proxy handlers for ns, getters and actions * Set scope inside `evaluate` * Refactor proxy handlers * Improve types a bit * Catch errors in async actions * Implement stacks for scopes and namespaces * Implement `getElement` * Change directives object structure * Remove unnecessary import * Implement private stores * Return value from sync actions * Minor optimizations and improved comments * Don't use async inside `data-wp-watch` * Use a single Provider in context directive * Remove DeepPartial type * Do not check if element exists * Add the `current` prop of state inside the scope * Move getters outside scope * Fix wc-key assignment * Fix missing `navigate` in directives * Fix namespace not being picked in the same element * Deep merge raw stores instead of proxied ones * Fix namespace assignment * Allow forward slashes in namespaces * Migration of Product Collection and Product Button blocks to the new `store()` API (#11558) * Refactor Product Button with new store() API * Use `wc_initial_state` in Product Button * Fix namespace * Remove unnecessary state * Test namespaces in directive paths * Add test context with namespace * Simplify woo-test context * Move addToCart and animations to a file * Do not pass `rawStore` to `afterLoad` callbacks * Move callbacks and actions back to the main file Because the animation was broken. * Remove selectors in favor of state * Use default ns in `getContext` for state and actions * Remove `afterLoad` callback * Remove unnecessary ns * Fix getContext in add-to-cart * Replace namespace and delete unnecessary store * Pass context types only once * Use an alternative for requestIdleCallback * Add previous react code for notices * Add namespace to Product Collection block * Replace getTextButton with getButtonText * Add block name to the ProductCollection namespace * fix style HTML code * Remove circular deps error on the Interactivity API * Product Gallery block: Migrate to new Interactivity API store (#11721) * Migrate Product Gallery block to new Interactivity API store * Fix some references * Add missing data-wc-interactive * Fix an additional namespace * Remove unnecessary click handler * Dialog working * Refactor action names * Reindex PHP array There was some missing indexes, which turned the array into an object in JS. * Remove unused event handlers * Move next/previous logic to external function * Move StorePart util to the types folder * Rename namespace to `woocommerce/product-gallery` * Undo product collection namespace renaming * Remove unnecessary namespace * Don't hide the large image on page load * Minor refactorings * Fix eslint error * Fix php cs errors with spacing and double arrows alignment * Disable no-use-before-define rule for eslint * Disable @typescript-eslint/ban-types rule for eslint * Fix parsed context error in e2e tests * Fix context parser for Thumbnail image * Move store to the top of the frontend file * Add interactivity api utils to the @woocommerce/utils alias * Replace deprecated event attribute --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: David Arenas <david.arenas@automattic.com> Co-authored-by: roykho <roykho77@gmail.com> --------- Co-authored-by: David Arenas <david.arenas@automattic.com> Co-authored-by: Luigi Teschio <gigitux@gmail.com> Co-authored-by: Alexandre Lara <allexandrelara@gmail.com> Co-authored-by: roykho <roykho77@gmail.com> * Fix error when closing product gallery dialog with keyboard escape key * use wc_initial_state instead of wc_store --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Luigi Teschio <gigitux@gmail.com> Co-authored-by: Alexandre Lara <allexandrelara@gmail.com> Co-authored-by: roykho <roykho77@gmail.com>
What
Migrates both the Product Collection and the Product Button block to the new store() API.
Resources:
store()
API #11065store()
API #11071Why
Because we are switching from the old
store()
API to the newstore()
API, and we need to migrate all the blocks at once.Testing Instructions
trunk
Screenshots or screencast
I made a video to explain the migration of this block:
https://www.loom.com/share/cbe30d0c820e4f99a3e6697a5f85b759
WooCommerce Visibility
Required:
Checklist
Required:
[type]
label or a[skip-changelog]
label.Conditional:
[skip-changelog]
label is not present).Changelog
Migrate the Product Button to the new
store()
API of the Interactivity API.